Support initial modelVersion in Check Saved Objects CI step#248443
Support initial modelVersion in Check Saved Objects CI step#248443gsoldevila merged 4 commits intoelastic:mainfrom
modelVersion in Check Saved Objects CI step#248443Conversation
|
Pinging @elastic/kibana-core (Team:Core) |
🔍 Preview links for changed docs |
25adc96 to
0f96878
Compare
modelVersion in Check Saved Objects CI step
| if ( | ||
| // a SO type with no migrations: and only a single modelVersion: is, by definition, | ||
| // compatible, as we only allow schema definitions in the initial modelVersion. | ||
| (currentVersion !== '10.1.0' || latestVersion !== '0.0.0') && |
There was a problem hiding this comment.
This is the first key change. I update the document_migrator logic to not fail in the 10.1.0 vs 0.0.0 scenario.
| if (mvs.length === 1 && mv.changeTypes.length) { | ||
| // Do NOT allow changes in the first (initial) modelVersion, only schema additions. | ||
| // This guarantees rollback safety towards previous versions. | ||
| throw new Error( | ||
| `❌ The new model version '${mv.version}' for SO type '${name}' is defining mappings' changes. For backwards-compatibility reasons, the initial model version can only include schema definitions.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Second key change: Not allowing changes: [...] in the initial model version.
There was a problem hiding this comment.
For these errors it might be nice to link to the docs
rudolf
left a comment
There was a problem hiding this comment.
Implementation looks good, left some nits for the docs
| // console.log('SNAPSHOT', JSON.stringify(ctx.to, null, 2)); | ||
| // throw new Error('STOP'); |
There was a problem hiding this comment.
| // console.log('SNAPSHOT', JSON.stringify(ctx.to, null, 2)); | |
| // throw new Error('STOP'); |
| }; | ||
| ``` | ||
|
|
||
| ### Defining model versions for existing Saved Objects |
There was a problem hiding this comment.
I think it would be worth explaining here that you'll need to do two PR's one per serverless release. Otherwise they might commit their model version 1, CI passes, then commit model version 2 with changes and CI again complains.
There was a problem hiding this comment.
I think we have a bit of a situation here. At the moment:
-
The
pull_requestpipeline is only testing against merge-base commit. Otherwise, there are multiple PRs that stem from branches that are older than current serverless release, which fail due to discrepancies in SO definitions. Good examples of this could be large PRs or feature branches. Imagine someone delivers a new modelVersion for a SO type, it gets released in Serverless. At that point, the CI check will fail for any branches that don't have that new modelVersion, as it will believe that the PRs are "deleting" the missing modelVersion. Well, not all PRs, only those that are making changes to SO definitions. -
OTOH, the
on_mergepipeline is testing against the current serverless release. The problem is that failures at this point are not very actionable.
Thus, for a scenario like the one you describe (aka someone creating 2 modelVersions within the same Serverless release), the 2nd modelVersion PR would pass pull_request CI and would fail in the on_merge pipeline.
We could test against current Serverless release in the pull_request pipeline, but that would mean that folks updating SO types would have to frequently rebase their PRs for CI to work.
There was a problem hiding this comment.
Maybe we could run the check twice in the pull_request pipeline, once against the merge-base commit, and once against the current Serverless release SHA.
I think this kinda takes us to the 1 script vs 2 scripts discussion, in the sense that:
- Some errors are caused by the SO type owner, and thus, actionable (e.g. those happening when testing against
merge-base). - Other errors are not the users' fault, e.g. someone else introduced a SO type in Serverless that my branch does not have yet (happening when testing against current serverless release). The only action devs can take here is rebasing, or waiting for next serverless release, but these aren't changes that are inherent to the SO definitions.
I wonder if having two separate checks would make sense:
- Leave current check as-is: Are MY SO types' changes valid?
- Introduce a new check against current Serverless release SHA: Is my PR safe to merge, from the current Serverless release standpoint?
NB that if we don't check against merge-base, and we simply run the check against current Serverless release SHA, we can end up having errors in SO and not knowing whether they are caused by the users' changes (1) or by the PR branch divergence (2).
| if (mvs.length === 1 && mv.changeTypes.length) { | ||
| // Do NOT allow changes in the first (initial) modelVersion, only schema additions. | ||
| // This guarantees rollback safety towards previous versions. | ||
| throw new Error( | ||
| `❌ The new model version '${mv.version}' for SO type '${name}' is defining mappings' changes. For backwards-compatibility reasons, the initial model version can only include schema definitions.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
For these errors it might be nice to link to the docs
db616f8 to
b97a264
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Unknown metric groupsReferences to deprecated APIs
History
|
…tic#248443) ## Summary Closes elastic#248231 The PR updates the `Check Saved Objects` CI step logic to support the definition of the first (initial) modelVersion `"1"`, by doing the following 2 things: * Not allowing mappings changes in the initial model version `10.1.0`. * Updating the document migrator logic to consider `10.1.0` compatible with `0.0.0`. Without this change, the CI check fails irremediably whenever we are introducing the first update (first _modelVersion_) for a given SO type. When stumbling upon such object after a rollback, the SOR APIs will not know how to handle it, and fail with an error of the form: ``` ERROR Error: Document "1281cd3a-293a-4415-ad71-64e8f79e8086" belongs to a more recent version of Kibana [10.1.0] when the last known version is [0.0.0]. ```
Summary
Closes #248231
The PR updates the
Check Saved ObjectsCI step logic to support the definition of the first (initial) modelVersion"1", by doing the following 2 things:10.1.0.10.1.0compatible with0.0.0. Without this change, the CI check fails irremediably whenever we are introducing the first update (first modelVersion) for a given SO type. When stumbling upon such object after a rollback, the SOR APIs will not know how to handle it, and fail with an error of the form: